Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not insert a new job if PeriodicJobConstructor returns nil #567

Closed
wants to merge 2 commits into from
Closed

Do not insert a new job if PeriodicJobConstructor returns nil #567

wants to merge 2 commits into from

Conversation

semanser
Copy link
Contributor

@semanser semanser commented Aug 26, 2024 •
edited
Loading

This commit fixes an issue with the PerioridJobConstructor ignoring the return value. According to the docs, we should ignore the job is nil is returned.

Before:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x1089c1d1]

goroutine 470 [running]:
github.com/riverqueue/river.insertParamsFromConfigArgsAndOptions(0xc0007e2270, 0xc0003a3500, {0x0, 0x0}, 0x0)
        /Users/semanser/Programming/river/client.go:1218 +0x531
github.com/riverqueue/river.(*PeriodicJobBundle).toInternal-fm.(*PeriodicJobBundle).toInternal.func1()
        /Users/semanser/Programming/river/periodic_job.go:186 +0x4e
github.com/riverqueue/river/internal/maintenance.(*PeriodicJobEnqueuer).insertParamsFromConstructor(0xc0007e2270, {0x11a6dc58, 0xc000a3a550}, 0xc000392438, {0xc1ab4192dd537df8, 0xbb93aaf1, 0x12a4f360})
        /Users/semanser/Programming/river/internal/maintenance/periodic_job_enqueuer.go:413 +0xb2
github.com/riverqueue/river/internal/maintenance.(*PeriodicJobEnqueuer).Start.func1.2(0xc0007e2270, {0xc18a41c1?, 0x12a4f360?, 0x12a4f360?}, {0x11a6dc58, 0xc000a3a550}, 0xc000a27ee8, 0xc000a27f00)
        /Users/semanser/Programming/river/internal/maintenance/periodic_job_enqueuer.go:307 +0x1eb
github.com/riverqueue/river/internal/maintenance.(*PeriodicJobEnqueuer).Start.func1()
        /Users/semanser/Programming/river/internal/maintenance/periodic_job_enqueuer.go:321 +0x3d5
created by github.com/riverqueue/river/internal/maintenance.(*PeriodicJobEnqueuer).Start in goroutine 120
        /Users/semanser/Programming/river/internal/maintenance/periodic_job_enqueuer.go:212 +0xd6

After:

2024-08-26T16:19:33.510+0200    INFO    PeriodicJobEnqueuer: nil returned from periodic job constructor, skipping

@semanser
Do not insert a new job if PeriodicJobConstructor returns nil
968fced 
This commit fixes an issue with the PerioridJobConstructor ignoring the
return value. According to the docs, we should ignore the job is nil is
returned.
@bgentry
Copy link
Contributor

bgentry commented Aug 26, 2024

Odd, I could have sworn we had tests for this case. Maybe it was lost during a refactor somehow 🤔 thank you for submitting, will just need to look to see if I can figure out whether we have some existing broken tests for this or if it was simply never covered.

@bgentry
Copy link
Contributor

bgentry commented Aug 26, 2024

@semanser would you mind signing off on the project’s CLA as well? https://github.com/riverqueue/rivercla

@semanser semanser mentioned this pull request Aug 26, 2024
Merged
@brandur
Copy link
Contributor

brandur commented Aug 29, 2024

@semanser Could you take a look at the lint failures on this one? It seems like you might be able to get away without adding a TestJobArgs for the new test case.

@semanser
Copy link
Contributor Author

@brandur done!

@bgentry bgentry mentioned this pull request Aug 30, 2024
Merged
@bgentry
Copy link
Contributor

bgentry commented Aug 30, 2024

@semanser I brought over this PR into #572, added a lint fix for a pre-existing lint issue, and added additional high level coverage at the client level to be confident this doesn't break in the future. Thank you again for the fix! ✌️

@bgentry bgentry closed this in #572 Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@semanser @bgentry @brandur